Skip to content

Scala 2.12.0-RC1 support. #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Scala 2.12.0-RC1 support. #177

wants to merge 2 commits into from

Conversation

paulp
Copy link

@paulp paulp commented Sep 8, 2016

No description provided.

@paulp paulp mentioned this pull request Sep 8, 2016
@ghost
Copy link

ghost commented Sep 10, 2016

As per gitter chat, I will re-introduce 2.10 to this PR so we can now cover 2.10. 2.11 and 2.12. I've donw the work already, but I really need to restructure some bit. Just a heads-up on what I'm doing

@ghost
Copy link

ghost commented Sep 10, 2016

Done. Here is a git diff https://gist.github.com/BennyHill/401b44ee62f123f23b52ad56d98e5db3

Or I could push tp @paulp 's repo branch, but the changes are done.

@paulp
Copy link
Author

paulp commented Sep 11, 2016

@BennyHill I applied your patch.

@ghost
Copy link

ghost commented Sep 11, 2016

@paulp 👍

@sksamuel
Copy link
Member

Thanks for the PR @paulp @BennyHill
We have a branch ready to go for 1.2 to address some long standing issues. I'd like to wait on this PR until 1.3 which will support scala 2.12 and will be released immediately-ish after, is that's ok.
@gslowikowski

@rorygraves
Copy link
Contributor

@sksamuel Where is the 1.2 branch? Can we get it out and then release a 1.3-RC1 to get all of @paulp and @BennyHill s changes in?

@sksamuel
Copy link
Member

@gslowikowski has it locally.

@gslowikowski
Copy link
Member

I will start merging changes today

@ghost
Copy link

ghost commented Sep 17, 2016

👍

gslowikowski added a commit to gslowikowski/scalac-scoverage-plugin that referenced this pull request Sep 17, 2016
…ly with 2.11).

Details:
- sbt version upgraded from 2.13.11 to 2.13.12,
- scala version upgraded from 2.11.7 to 2.11.8,
- scalatest version upgraded from 3.0.0-M15 to 3.0.0,
- unneeded 'javaOptions += "-XX:MaxMetaspaceSize=2048m' settings removed from build,
- unneeded EnvSupport class removed from build,
- tests made compatible with Scala 2.10,
- TravisCI configured for Scala 2.10.6 and 2.11.8.

Some fixes borrowed (with modifications) from scoverage#177, thank you @paulp and @BennyHill.
@gslowikowski
Copy link
Member

I've just created and merged #179. This PR has conflicts now.

I would like it to be split into separated PRs for:

  • logging (dependencies) improvement - scala-logging 3.5.0 is available, use this version instead of a snapshot; is scala-logging-slf4j not required anymore?
  • maybe separate one for other build improvements - if there are not many, they can be added to previous one
  • Scala 2.12 support - only the logic related to this problem; I would like this PR to be easy for anyone to read and understand, what was required for Scala 2.12 integration.

Will you prepare there PRs or should I do it myself?

@paulp
Copy link
Author

paulp commented Sep 17, 2016

All the changes in my original PR (41784d8) are to work with scala 2.12. In some cases "build improvement" was the most expeditious route to that goal.

@ghost ghost mentioned this pull request Sep 18, 2016
@gslowikowski
Copy link
Member

There is no com.typesafe.scala-logging:scala-logging_2.10:3.5.0 dependency. How will it work with Scala 2.10? It's used in ScoverageCompiler.scala.

@ghost
Copy link

ghost commented Sep 18, 2016

@gslowikowski From gitter:

So: compiling and testing with coverage on 2.10.6 does indeed fail to compile
But... removing logging from CreditEngine fixes the issue
logging is only used in that class, and the logging lib was compiled with 2.10.4 and so should/might explain this
It is an old logging lib, unfortunately newer version (3.x) are only compiled with 2.11.x
mixing in the StrictLogging trait is ok, it's the actual call to logger.debug that is the issue
I think a good test would be if I compile the old logging lib with 2.10.6 and see if that works.
Actually, see lightbend-labs/scala-logging#30
This is an open issue >18 months at lightbend so maybe best just to dump this lib completely

This lib is only used as a test-case, not for real logging. Hence we thought best to ditch it.

@gslowikowski
Copy link
Member

If you think it's not needed, we can remove it.

@gslowikowski
Copy link
Member

Some code (required to run tests on Scala 2.10) from this PR was already copied by me and merged.

I would like to see two separate PRs created from this one:

  • one with different build improvements (like removing scala-logging, code refactoring, etc.), preparation for Scala 2.12,
  • second one with pure Scala 2.12 support added.

Can you do it?

@gslowikowski
Copy link
Member

Some questions from me:

  1. In plugin test classes (LocationCompiler.scala and ScoverageCompiler.scala) overridden runsRightAfter method was added in three classes extending PluginComponent (see here , here and here). Is it required? What is it for? Tests work for me without it.
  2. In ScoverageCompiler.scala in ScoverageTestStoreComponent.runsAfter method "dce" was changed to "jvm" (see here), but there is no "jvm" phase in Scala 2.11 (see here). It's defined in 2.10, removed in 2.11 and restored in 2.12.
    "jvm" removal - commit authored by @paulp
    "jvm" restored - commit
    BTW, it was never removed from Scalac.scala

@gslowikowski
Copy link
Member

I've created #181 with different build improvements, some copied from this PR and a proposal of "clean" Scala 2.12 support introducing changes in this branch (one commit).

I'm waiting for answers to my questions above first.

@paulp
Copy link
Author

paulp commented Sep 22, 2016

I'm going to defer to others here.

@gslowikowski
Copy link
Member

@sksamuel, can you take a look and try to answer my questions above?

@rorygraves
Copy link
Contributor

@gslowikowski There is definately a jvm phase in 2.11 - type scalac -Xshow-phases but it has some special handling

@paulp
Copy link
Author

paulp commented Sep 23, 2016

@rorygraves Correct, the line which @gslowikowski linked to

    // val jvmPhase                     = phaseNamed("jvm")

has no bearing on the existence of the phase.

@gslowikowski
Copy link
Member

I've updaged my branch https://github.com/gslowikowski/scalac-scoverage-plugin/commits/scala212-simplified-proposal
Tests work for all Scala versions. You can test it.

@gslowikowski
Copy link
Member

Can you update this PR?

Restore these two accidentally removed tests:

// https://github.com/skinny-framework/skinny-framework/issues/97
test("macro range positions should not break plugin") {
val compiler = ScoverageCompiler.default
compiler.addToClassPath("org.slf4j", "slf4j-api", "1.7.7")
compiler.addToClassPath("com.typesafe.scala-logging", "scala-logging-api_" + ScoverageCompiler.ShortScalaVersion, "2.1.2")
compiler.addToClassPath("com.typesafe.scala-logging", "scala-logging-slf4j_" + ScoverageCompiler.ShortScalaVersion, "2.1.2")
compiler.compileCodeSnippet( """import com.typesafe.scalalogging.slf4j.StrictLogging
|
|object MacroTest extends StrictLogging {
| println("Hello")
| logger.info("will break")
|} """.stripMargin)
assert(!compiler.reporter.hasErrors)
assert(!compiler.reporter.hasWarnings)
}

test("plugin should not instrument expanded macro code github.com/skinny-framework/skinny-framework/issues/97") {
val compiler = ScoverageCompiler.default
compiler.addToClassPath("org.slf4j", "slf4j-api", "1.7.7")
compiler
.addToClassPath("com.typesafe.scala-logging",
"scala-logging-api_" + ScoverageCompiler.ShortScalaVersion,
"2.1.2")
compiler
.addToClassPath("com.typesafe.scala-logging",
"scala-logging-slf4j_" + ScoverageCompiler.ShortScalaVersion,
"2.1.2")
compiler.compileCodeSnippet( """import com.typesafe.scalalogging.slf4j.StrictLogging
|class MacroTest extends StrictLogging {
| logger.info("will break")
|} """.stripMargin)
assert(!compiler.reporter.hasErrors)
assert(!compiler.reporter.hasWarnings)
compiler.assertNoCoverage()
}

@paulp
Copy link
Author

paulp commented Sep 24, 2016

No, I've put what time into it I can.

@rorygraves
Copy link
Contributor

@gslowikowski based on the the fact that half of these changes have been picked/merged separately I suggest you close this PR and manually import any other changes you are going to take.

@gslowikowski
Copy link
Member

@rorygraves I didn't want to take this PR over, but at the end created #182 from my branch (which I hoped would serve for discussion about improving this one).

There are still areas for test harness improvements, but new release is more desired now. Improvements can be made at any time later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants